fix(checkbox): show labels after page navigation#31062
fix(checkbox): show labels after page navigation#31062OS-jacobbell wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
brandyscarney
left a comment
brandyscarney
left a comment
There was a problem hiding this comment.
Looks good! Please update the PR title before merging. 🙂
OS-jacobbell
changed the title
ShaneK
left a comment
ShaneK
left a comment
There was a problem hiding this comment.
Looking really good! A couple of minor concerns, though
| }); | ||
| this.validationObserver = new MutationObserver((mutations) => { | ||
| // Watch for label content changes | ||
| if (mutations.some((mutation) => mutation.type === 'characterData')) { |
There was a problem hiding this comment.
This only catches mutations to existing text nodes (.data changes). If the framework replaces slot children entirely, like an *ngIf toggling a label, or el.textContent = 'new' which removes all children and creates a new text node, that's a childList mutation and this won't fire. The original onSlotchange covered both cases.
Should childList: true go into the observe config alongside characterData: true? The condition here would just become mutation.type === 'characterData' || mutation.type === 'childList'. createSlotMutationController uses childList for the same reason.
Also, radio.tsx and toggle.tsx have the same get hasLabel() getter pattern that checkbox had before #30980, so they're vulnerable to the same bug. Probably follow-up tickets rather than scope creep here, but good to track.
| attributes: true, | ||
| attributeFilter: ['class'], | ||
| characterData: true, | ||
| subtree: true, |
There was a problem hiding this comment.
subtree: true applies to all observation types, not just characterData. So the attributeFilter: ['class'] now fires for descendant class changes too, not just the host element. It's harmless since the handler filters by mutation.type, but you could tighten the attributes branch with mutation.target === el to match the original observer's scope.
3 participants
Issue number: resolves #31052
What is the current behavior?
After a page navigation, ion-checkbox's
onslotchangeevent fires before the element'stextContenthas been updated. It is called again aftertextContentbecomes readable on Safari, but is not called again after thetextContentbecomes readable on Chrome and Firefox.What is the new behavior?
MutationObserverinstead ofonslotchangeand fires specifically on character data changes. This ensureshasLabelContentis up to date.MutationObserverdoes not fire on load, sohasLabelContentis initialized inconnectedCallbackDoes this introduce a breaking change?